New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add WithFiles to Directory and Container #6556
Conversation
Since this is a core change you only run changie in the root of the repo, not the sdks. |
Yeah, I was wondering why you didn't 🙂 |
a773d0a
to
b94e2c7
Compare
refactored and added the changeset :) |
|
||
var err error | ||
for _, file := range src { | ||
dir, err = dir.WithFile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is fine today, I think we could potentially invert this - have WithFile
call WithFiles
.
We could have mergeStates
take multiple source inputs, which would potentially be much more efficient (translating to one MergeOp
instead of lots of individual ones). That said, this is a trickier optimization (and I think is also somewhat blocked on #6073).
Just a note, no action required here (but we could potentially leave a TODO
comment for follow-up).
docs/docs-graphql/schema.graphqls
Outdated
"""Identifiers of the files to copy.""" | ||
files: [FileID!]! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Identifiers of the files to copy.""" | |
files: [FileID!]! | |
"""Identifiers of the files to copy.""" | |
sources: [FileID!]! |
We should mirror the structure of withFile
.
withFile
➡️withFiles
source
➡️sources
Thanks for the contribution @tomasmota! 🎉 Sorry for the nitpicky-comments, only just got round to reviewing this 😢 |
The more thorough the review the more I learn, so nitpick away! I'll rename and add the TODO :) |
Signed-off-by: tomasmota <tomasrebelomota@gmail.com>
Thanks @tomasmota! |
Fixes: #6475
The issue mentioned only Directory, but I also added it to Container, not sure that was a good idea. Also, this is my first contribution so I might be missing a few things.
With regards to the sdk changelog, it seems like that is something I should do after creating the PR? go through every sdk and run it?
Also a question: in Directory WithFiles, I wasn't sure whether or not I should have re-used WithFile, some feedback on that would be great :)